Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ecpair): ensure last non-trivial input pair goes to ML+finalexp circuit #524

Merged
merged 24 commits into from
Jan 16, 2025

Conversation

ivokub
Copy link
Contributor

@ivokub ivokub commented Jan 10, 2025

This PR fixes ECPAIR glue in case when the last input pair is not non-trivial (either (0,0) or (0, Q) or (P, 0)). Previously, two things happened when the last input was half-trivial (only G2 membership check i.e. (0, Q)) - it was sent for the G2 membership circuit for testing (as requested by the arithmetization) and we didn't mark the last non-trivial input to go to the ML+finalexp circuit (as it was decided by the input pair ID relative to the total number of pairs).

To overcome that, we now in the glue index the input pairs differently. Instead of following the input pair IDs coming from the arithmetization, we count it ourselves only for the inputs which go to the pairing circuits. And for the "total pairs" we count only non-trivial pairs (as the ML of trivial pairs is 1 and doesn't change the accumulator. What only matters is that they are valid i.e. belonging to the correct subgroups).

To make it work, we had to add a constraint to ensure the total number of pairs was correctly set. But the constraints to ensure sequential counting of the non-trivial pairs still follows from the previous constraints (we check that after every 60 rows the input ID increases).

Additionally, we had to ensure the accumulator consistency correctness. When we have only one valid pair, then we only call ML+finalexp circuit. In this case we cannot check the consistency of the accumulator, but instead have to check that the previous accumulator going to the ML+finalexp circuit is correctly initialized.

Another fix I had to implement is to allow to have pairing circuit inputs and G2 membership inputs in the same rows (see 60ee51c). Previously when a row was an input to the G2 membership circuit, then the index constraint was not enforce, but should have been.

Finally - added regression test for a transaction which incurred the bug.

Also did some cosmetic changes:

  • generated testdata modules for some test cases
  • unified the testdata modules to have same order of columns
  • added option InHex to csvTraces.FmtCSV method to force dumping also small outputs in hex. This is mainly to have minimal diff of the current testcase modules.
  • defined a utility method for dumping the testdata module. It is useful for creating more regression tests.

On top of that, I added a comprehensive test case generator which generates all possible 1-5 input pair combinations for the pairing check. Currently the tests are skipped as it generates about 65000 test cases, but can be manually called.

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • I have informed the team of any breaking changes if there are any.

@ivokub ivokub added bug Something isn't working Prover Tag to use for all work impacting the prover P1: High Issue priority: high labels Jan 10, 2025
@ivokub ivokub self-assigned this Jan 10, 2025
Copy link

cla-assistant bot commented Jan 10, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Jan 10, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ivokub ivokub changed the title Fix/ecpair last finalexp fix(ecpair): ensure last non-trivial input pair goes to ML+finalexp circuit Jan 10, 2025
@ivokub ivokub had a problem deploying to docker-build-and-e2e January 10, 2025 01:48 — with GitHub Actions Error
@ivokub ivokub force-pushed the fix/ecpair-last-finalexp branch from aad22d2 to 41674db Compare January 15, 2025 11:14
@ivokub ivokub had a problem deploying to docker-build-and-e2e January 15, 2025 11:16 — with GitHub Actions Error
@ivokub ivokub had a problem deploying to docker-build-and-e2e January 15, 2025 11:19 — with GitHub Actions Error
@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.81%. Comparing base (fe967c7) to head (cc93af0).

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #524   +/-   ##
=========================================
  Coverage     68.81%   68.81%           
  Complexity     1165     1165           
=========================================
  Files           327      327           
  Lines         13030    13030           
  Branches       1311     1311           
=========================================
  Hits           8966     8966           
  Misses         3513     3513           
  Partials        551      551           
Flag Coverage Δ *Carryforward flag
hardhat 98.74% <ø> (ø)
kotlin 66.43% <ø> (ø) Carriedforward from 5150b25

*This pull request uses carry forward flags. Click here to find out more.

@ivokub ivokub temporarily deployed to docker-build-and-e2e January 15, 2025 11:43 — with GitHub Actions Inactive
@ivokub ivokub marked this pull request as ready for review January 15, 2025 19:48
@ivokub
Copy link
Contributor Author

ivokub commented Jan 15, 2025

@AlexandreBelling - PR is now ready, I have run the test suite locally on my machine and passed cleanly.

@gusiri gusiri had a problem deploying to docker-build-and-e2e January 16, 2025 10:46 — with GitHub Actions Error
@gusiri
Copy link
Contributor

gusiri commented Jan 16, 2025

@AlexandreBelling Enabled ecpair in zkevm.go

@ivokub ivokub temporarily deployed to docker-build-and-e2e January 16, 2025 10:59 — with GitHub Actions Inactive
@ivokub
Copy link
Contributor Author

ivokub commented Jan 16, 2025

One more thing - we merged the optimization to gnark to assert G2 membership already inside the Miller loop computation (see Consensys/gnark#1387). It saves (depending on the number of ML instances per circuit) 13-15% constraints. It requires updating gnark dependency and I haven't run all the test suite with it.

Should I do it in a separate PR (and waiting 24h for the test suite to run) or add the gnark dependency update here?

@AlexandreBelling
Copy link
Contributor

If it's only a performance thing we can wait a few days. The point for us here is to add back the pairings in prod quickly to validate beta-v1.

@ivokub
Copy link
Contributor Author

ivokub commented Jan 16, 2025

If it's only a performance thing we can wait a few days. The point for us here is to add back the pairings in prod quickly to validate beta-v1.

Alright - then it is good from my side.

@ivokub ivokub merged commit 05d3ee2 into main Jan 16, 2025
24 checks passed
@ivokub ivokub deleted the fix/ecpair-last-finalexp branch January 16, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1: High Issue priority: high Prover Tag to use for all work impacting the prover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants